Copy States Optimization #297
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 94.64% 96.22% +1.57%
==========================================
Files 9 9
Lines 654 715 +61
==========================================
+ Hits 619 688 +69
+ Misses 35 27 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
try fixing benchmarking upgrade manifest add Test(try fixing benchmarking) let ci refresh manifest Revert "let ci refresh manifest" This reverts commit 5385785. try fixing benchmarking
This reverts commit 2b10cee.
Angeladadd
force-pushed
the
cgsun/copy_states_refine
branch
from
8ac5abb to
273cde9
Compare
There was a problem hiding this comment.
Hi Chenge, I hope you are doing well. Sorry it's taken so long to review your code after marking the report. I think this is very good work and should definitely get merged into the upstream repo.
I'd like to have a few clarifying comments in some places that I've highlighted in my review comments. I think overall the optimisations you made should be the default behaviour, rather than something the user has to switch on. This would especially simplify the copy_states! and copy_states_dedup! functions that duplicate some code at the moment.
If Matt and Mose could also take a look at this, that would be great!
There was a problem hiding this comment.
Does this notebook reproduce the plots of your report? It's really nice to have it if it does! It could use some plain text description of what it's doing.
| # Verify BLAS implementation is OpenBLAS | ||
| @assert occursin("openblas", string(BLAS.get_config())) | ||
|
|
||
| # Set size of thread pool for BLAS operations to 1 | ||
| BLAS.set_num_threads(1) |
There was a problem hiding this comment.
This feels sensible. We don't want BLAS oversubscribing threads. Could you put a comment here to explain why we require OpenBLAS?
src/params.jl
Outdated
| particle_save_time_indices::V = [] | ||
| seed::Union{Nothing, Int} = nothing | ||
| n_tasks::Int = -1 | ||
| optimize_copy_states::Bool = false |
There was a problem hiding this comment.
Since you demonstrated this works well, I would like to have it true by default.
| @@ -1,5 +1,6 @@ | |||
| [deps] | |||
| ChunkSplitters = "ae650224-84b6-46f8-82ea-d812ca08434e" | |||
| Dates = "ade2ca70-3891-5945-98fb-dc099432e06a" | |||
There was a problem hiding this comment.
I don't see this being used anywhere? Do we need the test for stale dependencies @giordano
src/utils.jl
Outdated
| dedup::Bool = false | ||
| ) where T | ||
|
|
||
| if dedup | ||
| return copy_states_dedup!(particles, buffer, resampling_indices, my_rank, nprt_per_rank, to) | ||
| end |
There was a problem hiding this comment.
There's a bit of code duplication here that is not ideal, and I think is confusing GitHub in showing the diff. I would be happy with replacing the old copy_states! with the new deduplicating version entirely. I think you showed the overhead of removing the duplicates is small in all realistic cases. That would make the code easier to read and maintain in the future.
There was a problem hiding this comment.
I'm not sure I understand why the timer object is in the arguments of this function. If I still remember how this works, in the main run_particle_filter function the timer will be updated by the @timeit_debug macro in the calling function and returning it as an argument is redundant.
There was a problem hiding this comment.
The timer object is in the arguments because we have a separate testing script for the copy_states! function.
2 participants
Description
This PR introduces a two-phase optimisation to address communication bottlenecks in the copy_states routine during distributed resampling:
Issue
#116
Testing